Skip to content

Add more comments #1993

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 31 commits into
base: main
Choose a base branch
from
Open

Add more comments #1993

wants to merge 31 commits into from

Conversation

Bashamega
Copy link
Contributor

related to #1940
Hello:)
In my last PR I have used MDN to generate comments for interface declarations, in this pr I add it for the properties.

Copy link
Contributor

Thanks for the PR!

This section of the codebase is owned by @saschanaz - if they write a comment saying "LGTM" then it will be merged.

@Bashamega
Copy link
Contributor Author

Bashamega commented Apr 23, 2025

Not ready for review
Ready for review

Copy link
Contributor

@saschanaz saschanaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm yet to review this, but please do not convert to sync APIs.

@Bashamega
Copy link
Contributor Author

I'm yet to review this, but please do not convert to sync APIs.

Sure

@Bashamega
Copy link
Contributor Author

Bashamega commented Apr 23, 2025

Hello @saschanaz
I have switched the old functions to async, but the new function is still sync because the emitter is not async. I tried switching it to async, but it broke.

@Bashamega
Copy link
Contributor Author

I have updated the context to not call the API in the emitter, but that cause the test to fail, I will fix it later

@Bashamega
Copy link
Contributor Author

Done @saschanaz

@Bashamega
Copy link
Contributor Author

Bashamega commented Apr 26, 2025

Hello @saschanaz, @jakebailey
Do you have any other change requests in this PR?

@Bashamega
Copy link
Contributor Author

Hello @saschanaz
I have updated the emitter but I think the formatted is better to be a separate issue

@Bashamega
Copy link
Contributor Author

I have updated the script to use promise @saschanaz

@Bashamega
Copy link
Contributor Author

Hello @saschanaz
Do you have any changes?

@Bashamega
Copy link
Contributor Author

Bashamega commented May 5, 2025

Hello @saschanaz
Sorry, I was very busy. It seems that the old comments.json file was using the same format that I am using. If that format is incorrect, we should create another issue for that.

@saschanaz
Copy link
Contributor

saschanaz commented May 5, 2025

Not sure I follow? Following the format of the current comments.json would be actually ideal, but I don't think this PR is currently doing that.

The comment.json:

{
  "InterfaceName": {
    "properties": {
      "property": {
        "foo": {
          "comment": "(comment)"
        }
      }
    }
  }
}

This PR right now:

{
  "InterfaceName.foo": "(comment)"
}

Which looks more concise but with extra conversion step to the comment.json format anyway. Since we are automating it here it should directly emit the comment.json format.

@Bashamega
Copy link
Contributor Author

Sorry, i got confused. I will fix it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants